feat(engine): add runtime Copilot model controls - #2246

Security review: no regressions found

Reviewed the full diff (docs/engine.md, agentic_pipeline.rs, ado_aw_marker.rs, engine.rs) introducing runtime Copilot model pipeline-variable overrides.

• Runtime model values pass a strict allowlist ([A-Za-z0-9._:-]) in both the bash preamble (engine.rs::runtime_model_preamble) and the aw_info.json writer, before reaching --model "$ADO_AW_EFFECTIVE_MODEL" (quoted) or JSON output — no shell/JSON injection path.
• The new variable names (ADO_AW_MODEL_AGENT_COPILOT, ADO_AW_MODEL_DETECTION_COPILOT, ADO_AW_DEFAULT_MODEL_COPILOT) are added to BLOCKED_ENV_KEYS, so untrusted workflow engine.env cannot override them.
• Explicit engine.model still takes precedence over runtime overrides, and no write-capable token or Stage 3 write path is touched.

No exploitable finding. Minor non-security note: the allowlist/precedence logic is duplicated between engine.rs and ado_aw_marker.rs — low risk of the two drifting out of sync over time, not blocking.

🔒 Security review by PR Security Reviewer · auto · 61.6 AIC · ⌖ 5.94 AIC · ⊞ 10.2K
_{Comment /review to run again}

Test Quality review

The new tests are well-scoped for verifying what strings appear in generated invocations/env/scripts, but they don't verify the runtime shell logic actually behaves correctly — every assertion is a contains() check on generated text, never an execution of the generated bash. Three findings posted inline:

1. copilot_detection_env's explicit-model omission path is untested (only the Agent-side copilot_env omission has a test), leaving an asymmetric gap.
2. The runtime-model precedence/validation shell logic (runtime_model_preamble) is only string-matched, never executed, so a broken fallback order or validation regex would pass all four new invocation tests.
3. The precedence/validation algorithm is independently duplicated between ado_aw_marker.rs's ado_aw_runtime_model (bash) and engine.rs's runtime_model_preamble (bash-generating Rust) with nothing checking they agree.

None of these are merge-blocking on their own — the string-based tests do catch gross omissions — but the actual security-relevant validation logic (rejecting invalid model characters before they reach an embedded shell command) ships with zero behavioral test coverage.

🧪 Test quality analysis by Test Quality Sentinel · auto · 143.7 AIC · ⌖ 6.34 AIC · ⊞ 9.8K
_{Comment /review to run again}

Rust review: COMMENT

Note: the rust-critic sub-agent failed to return output (model access error) on two attempts; findings below are from my own pass only.

network:
  allowed:
    - defaults
    - "releaseassets.githubusercontent.com"

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 128.9 AIC · ⌖ 1.96 AIC · ⊞ 10.1K
_{Comment /review to run again}

Compiler-contract review focused on drift and typed-contract invariants (not general Rust style, covered by other reviewers).

Checked and clean:

• Front-matter grammar: no new/renamed fields in src/compile/types.rs.
• BLOCKED_ENV_KEYS correctly extended with the three new ADO_AW_MODEL_* runtime variables, and a test (copilot_engine_env_rejects_user_runtime_model_var_override) proves engine.env cannot shadow them.
• EMIT_AW_INFO/RUN_AGENT shell changes stay inside the ShellScript/Binding/shell_script! framework — new externals: entries are declared for the three runtime model vars, and ENFORCE_BASH_LINT=1 cargo test --bin ado-aw compile::shell / --test bash_lint_tests both pass locally with the change in place.
• docs/engine.md documents the new Runtime model controls section, precedence order, and the Detection-engine interaction — matches the code.
• No public PipelineSummary/GraphSummary fields touched; aw_info.json's model/detection_model keys are optional strings already handled by src/audit/model.rs.
• No ado-script bundle sources or codegen inputs (filter_ir.rs, Fact) changed, so the bundle/codegen drift checks don't apply here.

One finding posted inline on src/engine.rs: the renamed detection_invocation_with_config orphaned the old invocation_with_config, which is now dead code hidden behind #[allow(dead_code)] instead of being deleted.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 200.2 AIC · ⌖ 1.92 AIC · ⊞ 11.3K
_{Comment /review to run again}

Compiler Contract Review

This is a large, well-executed refactor of the Copilot invocation path (shell-string --model/args to a typed JSON request/prepared/result protocol shared by Rust and a new copilot-controller/copilot-runner TS bundle pair). I checked for the contract-drift classes this review focuses on and found none outstanding:

• Bundle registry / ado_bundle.rs: CopilotController and CopilotRunner are registered in Bundle::ALL, have path entries, and are correctly included in the no-ADO-auth set (GitHub/local-only auth, not ADO REST).
• Codegen drift: no changes to filter_ir.rs / Fact in this diff, so types.gen.ts / fact-catalog.gen.json correctly did not move.
• Lock drift: all .github/workflows/*.md changes have matching .lock.yml diffs.
• AGENTS.md / docs sync: the new copilot-shared/controller/runner directories are documented in the architecture tree and in docs/ado-script.md's bundle list; docs/engine.md gained a full "Runtime model controls" section describing the new ADO_AW_MODEL_* variables and precedence; docs/audit.md documents the new aw_info.json fields (model vs detection_model, requested-vs-observed). All in sync with the code.
• Generated shell: RUN_AGENT/RUN_THREAT_ANALYSIS route the new trusted-preflight/model-result logic entirely through typed Bindings and a registered phases:/fragments: shell script (APPEND_AW_INFO_FIELD, COPILOT_TRUSTED_PREFLIGHT) -- no format!-built shell, no raw credential bindings.
• Secure newtypes: the new is_valid_command_path validation (rejecting ./../empty/doubled-slash segments) and validate_model_name are mirrored exactly in protocol.ts's COMMAND_PATTERN/MODEL_PATTERN + hasSafePathSegments, so the Rust and TS halves of this security boundary cannot drift.
• Prior review feedback: the previously-flagged dead invocation_with_config method and triplicated model-precedence/validation logic are gone in this diff -- functionality was consolidated into invocation_request_with_config / validate_model_name and the shared APPEND_AW_INFO_FIELD phase.

I built the compiler (cargo build), ran the full Rust suite (cargo test, all passing) plus the targeted engine::/validate:: modules and runtime_model_controls_compile_across_all_targets, and ran the new copilot-controller/copilot-runner/copilot-shared vitest suites (43/43 passing) and npm run typecheck (clean). No findings to post inline.

network:
  allowed:
    - defaults
    - "releaseassets.githubusercontent.com"
    - "spsprodeus21.vssps.visualstudio.com"

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 142.1 AIC · ⌖ 1.91 AIC · ⊞ 12.5K
_{Comment /review to run again}

TypeScript review — scripts/ado-script/ (new copilot-controller/copilot-runner/copilot-shared protocol)

Reviewed the new copilot invocation split (protocol.ts, copilot-controller/index.ts, copilot-runner/index.ts) plus their tests. ts-critic ran in the background and returned 5 findings; after triage, 2 were genuine and merge-worthy and are posted inline below, the rest (prompt-size guard, swallowed JSON.parse detail, broad argv-path trust) are advisory nitpicks not worth blocking on.

Strengths:

• The request/prepared/result schema validation in protocol.ts is thorough — unknown-key rejection, NUL-byte checks, path/command allowlisting via regex, and no any/non-null-assertion type-safety holes on externally parsed JSON.
• Signal forwarding and exit-code mapping in copilot-runner is carefully handled (settle guard against double-resolve, off cleanup of listeners).
• Good test coverage of model precedence (resolveRequestedModel), unresolved ADO macro detection, and the role-mismatch / missing-result paths in copilot-controller.

Findings (inline):

1. copilot-controller/index.ts:27 — write-ordering bug: result.json is written before prepared.json, which can leave a stale/misleading result visible to the trusted host after a crash between the two writes, contradicting the documented "commit result before exposing prepared" intent.
2. copilot-runner/index.ts:50 — self-removal of preparedPath/runnerPath happens before spawn is confirmed to succeed, so a spawn error event (binary missing, perms) loses the invocation audit trail that would otherwise help debug the failure.

Neither is merge-blocking on its own (both are narrow-window failure modes), but #1 in particular undermines the stated trust-boundary guarantee of the two-file protocol, so I'd like it addressed or the design rationale clarified before merge.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 154.9 AIC · ⌖ 8.49 AIC · ⊞ 11.3K
_{Comment /review to run again}

Security review — no blocking findings

Reviewed the diff against the threat model (compiler → generated-YAML injection, Stage 1/3 token isolation, AWF sandbox boundary, validation strength).

Net assessment: security-neutral to positive. The headline change — replacing the shell-embedded --prompt="$(cat ...)" engine invocation string with a copilot-controller/copilot-runner protocol that spawns Copilot via child_process.spawn(command, argv[]) — removes a shell-reinterpretation step for prompt/arg content and is a hardening, not a regression.

Specifics checked:

• is_valid_command_path (Rust) and isSafeCommand/hasSafePathSegments (TS protocol.ts) are tightened together: both now reject ./.. segments, empty segments, and trailing slashes — stricter than the prior character-class-only check.
• Bundle::CopilotController / Bundle::CopilotRunner are correctly placed in the no-SYSTEM_ACCESSTOKEN bucket in ado_bundle.rs (no ADO REST calls), preserving least privilege.
• The trusted controller/result files live under $(Agent.TempDirectory), outside AWF's automatic /tmp mount (per docs/network.md), matching the existing azure-wif-refresh isolation pattern. The controller source copied into the sandbox-visible path is deleted before AWF starts, and AWF hasn't executed yet at that point, so there's no attacker-controlled race window.
• ADO_AW_MODEL_AGENT_COPILOT / ADO_AW_MODEL_DETECTION_COPILOT / ADO_AW_DEFAULT_MODEL_COPILOT / COPILOT_MODEL were added to BLOCKED_ENV_KEYS, and a new check rejects --model in engine.args, closing the obvious bypass of the new precedence chain. buildChildEnvironment strips the runtime-override vars before spawning the real Copilot process.
• validate_model_name / MODEL_PATTERN are kept in sync between the Rust compiler and the TS protocol module.

Didn't find a traceable injection, token-exposure, or weakened-validation path introduced by this diff, so no inline comments. (Pre-existing review threads already cover the DRY/dead-code concerns in engine.rs and ado_aw_marker.rs — out of scope for a security pass.)

🔒 Security review by PR Security Reviewer · auto · 157.6 AIC · ⌖ 2.72 AIC · ⊞ 11.4K
_{Comment /review to run again}

Verdict: COMMENT — no new merge-blocking defects in the Rust changes

Reviewed the Rust-only portion of the diff (src/audit/**, src/compile/ado_bundle.rs, src/compile/agentic_pipeline.rs, src/compile/extensions/{ado_aw_marker,ado_script,exec_context,mod}.rs, src/compile/types.rs, src/engine.rs, src/validate.rs, tests/compiler_tests.rs).

rust-critic returned three findings; I verified each:

• HIGH — "untrusted model name interpolated into JSON via printf without escaping" (ado_aw_marker.rs): verified false positive. REQUESTED_MODEL is validated against ^[A-Za-z0-9._:-]+$ (validate_model_name in src/engine.rs, mirrored by MODEL_PATTERN in scripts/ado-script/src/copilot-shared/protocol.ts) both when the request is built and again when parseInvocationResult re-parses the controller's result file. A quote or backslash can never reach the printf call.
• LOW — blocking Path::is_file() inside async find_analyzed_outputs_dir: this mirrors the pre-existing identical pattern in otel.rs::locate_agent_output_file (not introduced by this PR) — consistent with current codebase convention, not a regression.
• LOW — is_valid_command_path has no explicit length cap: defense-in-depth suggestion only; no demonstrated exploit path since the character allowlist already excludes shell metacharacters. Not blocking.

My own pass over error handling, casts, cross-platform paths, determinism (HashMap/sort() usage in copilot_provider_env/copilot_detection_env), and the new set -eo pipefail / set +e exit-code plumbing in RUN_AGENT/RUN_THREAT_ANALYSIS didn't surface additional issues — the fail-closed ordering (trusted prepare runs under set -e before set +e) is correct and is exercised by the new tests. cargo build is clean.

The genuinely interesting architectural risks here — triplicated runtime-model precedence/validation logic across engine.rs, the EMIT_AW_INFO bash script, and the TS protocol, the copilot_detection_env/copilot_engine_env test-coverage asymmetry, and the invocation_with_config dead-code path — are already tracked in existing review threads on this PR, so I'm not duplicating them.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 401.6 AIC · ⌖ 2.27 AIC · ⊞ 11.3K
_{Comment /review to run again}

Compiler-contract review

Focused on drift (codegen/lock/smoke), front-matter/safe-output/IR schema contracts, generated-shell discipline, and docs sync. Did not duplicate the Rust/TS quality findings already in pr-review-comments.json (the copilot_detection_env/copilot_env gating-test gap, the triplicated validation-algorithm note, and the shallow contains() invocation tests — all still accurate as of this diff and worth addressing).

No new contract-drift findings. Specifically checked and clean:

• Codegen/lock/smoke drift: no filter_ir.rs/Fact changes in this diff, so types.gen.ts/fact-catalog.gen.json correctly didn't move. No .github/workflows/*.md changed without its .lock.yml. tests/smoke/runtime-model-*.md are markdown-only sources wired through one cases.json entry each (REGISTERED.md updated to explain the agentic-lane queue-time variable) — matches the lane/registration design, no reintroduced per-case ADO definitions.
• Front-matter grammar: no new/removed FrontMatter fields in types.rs (only two internal helper methods removed, both now dead after the exec_context_*_active/safe_outputs_summary_active/etc. flag removal) — no codemod needed.
• Duplicated-activation-predicate removal: this PR actually resolves a drift risk rather than introducing one — the pr_contributor_will_activate/manual_contributor_will_activate/etc. "MUST stay in lock-step" helper pairs between exec_context/mod.rs and AdoScriptExtension are deleted wholesale, replaced by always-staging the Copilot controller/runner bundles in Agent prepare. Good simplification.
• New Bundle::CopilotController/CopilotRunner: correctly added to ado_bundle.rs's "no ADO bearer" exemption list (GitHub/no-REST bundles) — consistent with their GitHub App / credential-free nature.
• Generated shell: RUN_AGENT/RUN_THREAT_ANALYSIS/RECORD_DETECTION_RUNTIME_MODEL all go through ShellScript with typed bindings; the JSON invocation request is correctly passed via Binding::document (not raw interpolation), and the shared APPEND_AW_INFO_FIELD phase is reused via phase_body() rather than re-implemented at each call site — avoids yet another drift copy.
• Docs sync: AGENTS.md architecture tree, docs/ado-script.md, docs/engine.md (new "Runtime model controls" section), and docs/audit.md (new aw_info.model/detection_model semantics + overlay behavior) are all updated and match the implementation (src/audit/model.rs, cli.rs's merge_detection_aw_info, otel.rs's observed-vs-requested model precedence) — verified with the corresponding new tests in each file.
• Trusted/untrusted boundary: the new copilot-controller/copilot-runner/copilot-shared split (prepare on host, run-only in sandbox, strict schema-v2 validation with rejectUnknown, atomic writes, self-removal of the prepared doc before exec) matches the AGENTS.md security note added in this same PR about never trusting /tmp artifacts across the AWF boundary — the controller copy and result live under $(Agent.TempDirectory), outside AWF's host /tmp mount, consistent with that rule.

Nothing here rises to REQUEST_CHANGES on compiler-contract grounds; the gating/test-coverage gaps already flagged by prior reviews remain the operative action items.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 129.7 AIC · ⌖ 18.1 AIC · ⊞ 12.5K
_{Comment /review to run again}

Security review: no regressions found

This diff is security-neutral to positive. The headline change — replacing the shell-interpolated --prompt="$(cat path)" invocation (embedded inside a single-quoted AWF -- command string) with a trusted copilot-controller/copilot-runner protocol that reads the prompt file and passes it as a literal spawn() argv element (no shell: true) — removes a shell-expansion surface for prompt/model content rather than introducing one.

Specific checks performed against the threat model in this repo's docs:

• Injection into generated pipelines: the new INVOCATION_REQUEST document is built via serde_json::to_string and embedded through the existing Binding::document heredoc binding (which asserts no credential content and no delimiter collision) — not string concatenation. command, prompt_path, and mcp_config_path are compiler-controlled, not operator/PR-controlled.
• Weakened validation: none found. is_valid_command_path (src/validate.rs) got stricter (now rejects .., empty segments, trailing slash, bare relative paths like bin/copilot), and a new validate_model_name / MODEL_PATTERN enforces ^[A-Za-z0-9._:-]+$ consistently in both src/engine.rs and scripts/ado-script/src/copilot-shared/protocol.ts. protocol.ts additionally rejects unknown JSON fields (rejectUnknown) and NUL bytes in every parsed document.
• Token scope / ado_bundle.rs: CopilotController/CopilotRunner are added to the bundle registry and correctly placed alongside the no-ADO-auth bundles (GithubAppToken, ApprovalSummary, etc.) rather than the SYSTEM_ACCESSTOKEN-projected set — consistent with them never making ADO REST calls.
• Trust boundary: the controller (trusted, runs outside AWF, install -m 0500) writes the sandbox-visible invocation document and is then deleted (rm -f "$COPILOT_CONTROLLER_SOURCE_PATH") before AWF starts; the runner (untrusted, runs inside AWF) only reads the prepared document, which has already been schema-validated and had the compiler-forbidden ADO_AW_MODEL_*/COPILOT_MODEL env keys stripped via buildChildEnvironment. Prepared files are atomically written (writeJsonAtomic, mode 0600, rename-after-write).
• Network/allowlist: no new domains added.
• Safe-output integrity: unaffected by this diff.

No merge-blocking findings. The existing unresolved github-actions[bot] review threads (duplicated validation logic across runtime_model_preamble/ado_aw_runtime_model/protocol.ts, and the invocation_with_config dead-code question) are code-quality/maintainability concerns, not security regressions, and are already tracked — not duplicating them here.

🔒 Security review by PR Security Reviewer · auto · 115.8 AIC · ⌖ 2.02 AIC · ⊞ 11.4K
_{Comment /review to run again}

Test Quality Review

This PR's test coverage is thorough. The new copilot-shared/protocol.ts module is exhaustively tested (strict schema validation, every rejection path, atomic-write behavior, argv/env construction including adversarial prompt content). copilot-controller/copilot-runner each cover their success, error, and signal-forwarding paths, including the security-relevant "does not expose sandbox run mode" / "does not expose controller modes" boundary tests. The engine.rs runtime-model precedence and gating logic — previously flagged in earlier review passes as having an untested Detection-side gap and weak substring-only invocation assertions — now has symmetric Agent/Detection tests with structured field assertions (copilot_detection_env_omits_runtime_model_vars_for_explicit_model, copilot_invocation_request_defers_runtime_model_resolution, etc.), and the previously-flagged invocation_with_config dead code no longer exists. The Rust ado_aw_marker.rs metadata-merge logic is well covered for value/key-collision edge cases (e.g. append_aw_info_field_does_not_confuse_value_with_model_key) and failure-without-exit-propagation. The new smoke cases follow the two-file convention correctly.

I found one minor untested branch (inline comment): the audit console renderer's new requested-vs-observed model split only has tests for the "differ" and "equal" branches, not the "observed model absent" branch described as a non-fatal case in the PR summary.

Non-blocking — approving the test quality here.

🧪 Test quality analysis by Test Quality Sentinel · auto · 213.9 AIC · ⌖ 1.7 AIC · ⊞ 11K
_{Comment /review to run again}

TypeScript review (scripts/ado-script/)

Reviewed the new Copilot invocation protocol (copilot-shared/protocol.ts, copilot-controller, copilot-runner) and the compiler-smoke-e2e queue-variable/model-assertion additions.

No high-confidence defects found:

• Error handling is sound: copilot-runner forwards SIGINT/SIGTERM/SIGHUP with proper cleanup, settles exactly once, and fails closed when self-removal throws.
• resolveRequestedModel validates runtime model values against a strict character class and never includes the rejected value in its thrown message (verified by test).
• Path/command validation (hasSafePathSegments, isSafeCommand) correctly rejects traversal segments, relative paths, and trailing slashes.
• writeJsonAtomic writes with 0o600 and renames, avoiding partial-write exposure.
• New branches in signals.ts/cases.ts/ado-rest.ts are covered by matching test cases.

A second pass via a dedicated hostile-review sub-agent independently reached the same conclusion (no findings).

No inline comments to post.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 235.3 AIC · ⌖ 2.62 AIC · ⊞ 11.3K
_{Comment /review to run again}